Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fix get-dag and add version=1 option #260

Merged
merged 2 commits into from
Oct 22, 2021
Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 21, 2021

I was just noticing in #246 that get-dag doesn't seem quite right so went about trying to address it, then in the process I'm realising that I don't have much use for CARv2 with much of the stuff I'm interested in and not having the option to make v1 CARs is a bit annoying, so I added a -version to it as well.

The current form seems to write out every matching node in a selector, not block, which will get pretty gnarly with complex blocks and a match-all and likely not what the user wants. This version uses the selector for the walk only (so it can be an explore, not a match) and uses the block loader to indicate what blocks are visited. It's not entirely unreasonable that a user may want to match individual nodes and turn them into blocks, but I think it's very unlikely desired behaviour and they're thinking in terms of whole blocks contained within the DAG described by a selector that will walk that DAG.

This needs tests, the sample v1 and wrapped v2 are kind of funky, with inlines and some weird stuff that I'm not sure how to account for properly in tests so I was thinking about making a simple DAG that can have selectors run over.

I also wanted to get the same thing for ipfs dag export which is exclusively CARv1 (atm anyway): ipfs/kubo#8506 - I was making way for selectors in there too but figure that maybe this is the best place to experiment with some of that crazy, including questions of what to do with excessively linked DAGs that make the traversal too onerous.

@rvagg rvagg requested a review from willscott October 21, 2021 10:52
@rvagg rvagg force-pushed the rvagg/cmd-get-dag-improve branch 3 times, most recently from b270be5 to daace98 Compare October 21, 2021 11:08
Base automatically changed from unixfscreate to master October 21, 2021 18:14
@willscott
Copy link
Member

i think this needs a rebase now. 🤭

Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good. Maybe update the flag name for index so that we're consistent in using version=n as the way to do this :) https://github.com/ipld/go-car/blob/master/cmd/car/car.go#L89-L92

cmd/car/get.go Outdated Show resolved Hide resolved
cmd/car/get.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Oct 22, 2021

wew, ended up bailing on a nasty rebase and cherry-picked my way out of it; should be good now, I've replaced -v1 with -version=n for the index command now too .. still needs tests and I'd like to get some in here but it's a lower priority than what I'm supposed to be doing so maybe not today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants